🔍 Rust PR Review

Summary: Solid overall — the three-layer design (marker extension → parse_marker_step → discovery) is clean and well-tested. A few concrete issues worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/ado/discovery.rs:441-444 — is_direct_match logic inversion

  if markers.len() != 1 {
      return markers.is_empty(); // returns true for 0 markers!
  }

  When called with 0 markers this returns true, which would classify a non-ado-aw definition as Direct. The current callsite in classify_definition guards against it, but this is a latent bug waiting for the next caller. Should be return false — 0 markers means "not ado-aw", not "direct match".

• src/ado/discovery.rs:502-513 — doc comment contradicts implementation for UnknownRequiredParams
  The doc comment says:

  UnknownRequiredParams is propagated with a marker-less MatchedDefinition because the user explicitly opted in and may want to act on it.

  But the code returns None for UnknownRequiredParams (same arm as NotAdoAw, UnknownForbidden, PreviewFailed). Either the behavior needs to match the comment (propagate it) or the comment is stale. Silently dropping UnknownRequiredParams definitions means a user running secrets set --all-repos gets no indication that some pipelines were skipped because they require parameters — which is surprising.

• src/ado/discovery.rs:563-575 — repo_url() missing percent-encoding

  Some(format!("{}/{}/_git/{}", self.org_url.trim_end_matches('/'), self.project, self.repo_name))

  ADO project names can contain spaces (e.g. "My Project"). The API returns repository.url as a URL where spaces are percent-encoded (My%20Project), but self.project stores the decoded string. After normalize_repo_url lowercases both sides, "my project" still won't equal "my%20project". DiscoveryScope::CurrentRepo (which is the scope when --source is used alone) would silently return no results for any project with spaces in its name. The rest of the codebase uses percent_encoding::utf8_percent_encode(&ctx.project, PATH_SEGMENT) for URL construction — same fix needed here.

🔒 Security Concerns

• src/compile/extensions/ado_aw_marker.rs:966-975 — single-quote injection in generated echo step
  The echo line uses single-quoting around the user-controlled source value:

  echo 'ado-aw metadata: source={source} version={version} target={target}'

  If a source path contains a ' (e.g. agents/foo's-agent.md), the generated bash is syntactically broken. normalize_source_path strips " but not '. Since this is a compiler-generated step for arbitrary user-provided paths, normalize_source_path should also strip or escape single quotes — or the echo should use double-quoting with the JSON-safe form.

⚠️ Suggestions

• src/detect.rs:1420,1428,1459 — stale #[allow(dead_code)] annotations
  MARKER_STEP_PREFIX, MarkerMetadata, and parse_marker_step are all actively consumed by src/ado/discovery.rs which this PR wires in. The "upcoming Preview-driven discovery" comments are no longer accurate. These attrs can be removed.

• src/secrets.rs:1762 — fast-path bypassed in discovery
  resolve_definitions_via_discovery is called with local_lock_paths: None, meaning even definitions with a matching local lock file will spend a Preview API round-trip. If the caller has a local path available (e.g. via --source pointing to a local file), threading it through would save calls for direct pipelines. Not a correctness issue, but could matter in large projects.

✅ What Looks Good

• The parse_marker_step design (line-by-line scan, skip-malformed, forward-compatible via #[serde(deny_unknown_fields)]-free deserialization) is exactly right for an embedded-in-YAML marker format.
• The 8-permit semaphore with ADO_AW_PREVIEW_CONCURRENCY override is thoughtful — big project operators have an escape hatch without a code change.
• The build_lock_path_map / normalize_ado_yaml_path key alignment is correct and tested.
• discovered_to_matched keeping the rest of the CLI commands unchanged is a clean adapter pattern.
• Test coverage is thorough — 6 unit tests for the parser, 4 integration tests across all 4 targets, 12 unit tests for scope/classification logic.

Generated by Rust PR Reviewer for issue #624 · ● 1.1M · ◷

🔍 Rust PR Review

Summary: Solid, well-tested feature addition — two real issues worth addressing before merge.

Findings

🔒 Security Concerns

• src/compile/extensions/ado_aw_marker.rs:1021-1029 — The echo line in the generated bash step embeds the user-controlled source path inside a single-quoted string: echo 'ado-aw metadata: source={echo_source} ...'. The bash_single_quote_escape correctly handles ' quoting, but if a file is named something like agents/##vso[task.setvariable variable=FOO]value.md, the ##vso[ sequence survives into stdout verbatim, and the ADO agent's logging-command scanner would process it as a task command. normalize_source_path strips " and \n/\r but not ##vso[. Suggest adding a ##vso[ strip/replace in normalize_source_path (or directly before embedding in the echo) — consistent with the sanitisation already applied to other user-facing YAML surfaces.

🐛 Bugs / Logic Issues

• src/ado/discovery.rs:465-492 (resolve_definitions_via_discovery) — The skip-counters (skipped_required_params, skipped_forbidden, skipped_failed) are incremented inside the .filter() closure before the source_filter check. When --source agents/foo.md is active, definitions with UnknownRequiredParams / Forbidden / PreviewFailed are counted even if they have no markers and are wholly unrelated to the requested template. The resulting warnings ("Discovery skipped N definitions whose Pipeline Preview requires templateParameters...") will mislead users into thinking they might be missing consumers of agents/foo.md when those N definitions were unrelated pipelines that simply failed. Fix: only count a definition toward skip totals when either source_filter is None, or when the definition does reference the target source (i.e., filter first, count second — or two-pass).

⚠️ Suggestions

• src/ado/discovery.rs:38 — #![allow(dead_code)] with the comment "Wired into CLI commands in workstream S" is stale: resolve_definitions_via_discovery and discovered_to_matched are both called from secrets.rs in this PR. The file-level suppression silently hides any future dead-code regressions. Remove it (or narrow to only the pub async fn preview_pipeline if that's genuinely only used in tests for now).

• src/main.rs:98-108 / src/secrets.rs:197-198 — --source alone (without --all-repos) silently activates DiscoveryScope::CurrentRepo. The CLI help text says "Activates the discovery code path; pairs with --all-repos..." but doesn't explain that without --all-repos it scopes only to the current repo's definitions. A user who runs secrets set GITHUB_TOKEN --source agents/foo.md and gets zero results because they're in a different repo will be confused. Consider adding [only searches definitions in the current repository; add --all-repos to search the full project] to the help string.

✅ What Looks Good

• The empirical grounding (live spike against def 2434) and its impact on the architectural decision (marker in step body, not top-of-file comment) is excellent engineering rigour — well documented in both the PR description and the module doc comments.
• bash_single_quote_escape + its dedicated test covering the '\'' idiom is exactly the right defensive approach; good test for single-quote in a path.
• parse_marker_step is robust: trims leading whitespace, skips non-JSON hits, ignores unknown future fields via serde defaults. The six unit tests cover the edge cases well.
• Fast-path short-circuit (local lock file → skip Preview call) with graceful fall-through when the file doesn't parse is a clean performance/resilience trade-off.
• discovered_to_matched correctly gates writes to only Direct | Consumer statuses, preventing accidental action on ambiguous (UnknownRequiredParams, Forbidden) definitions.

Generated by Rust PR Reviewer for issue #624 · ● 771.9K · ◷

🔍 Rust PR Review

Summary: Solid design with good test coverage — a few targeted fixes needed before merge.

Findings

🐛 Bugs / Logic Issues

• src/ado/discovery.rs:551 — source_filter is not normalized before comparison

  The marker's source field is stored in normalized form (via normalize_source_path: strips ./, converts \ to /, strips newlines). But the value from --source is passed through as-is:

  Some(src) => d.markers.iter().any(|m| m.source == src),

  A user passing --source ./agents/foo.md or --source agents\foo.md (Windows) silently matches nothing instead of finding the expected pipeline. The fix is to call normalize_source_path(Path::new(src)) on the filter value before the comparison, matching the exact normalization applied at compile time.

• src/ado/mod.rs:222 — misleading revision doc comment

  The field comment says "Used as a cache key by Preview-driven discovery so an unchanged definition is only previewed once per session", but discover_ado_aw_pipelines never reads revision and no caching logic exists anywhere in the PR. The comment will mislead future contributors looking for the caching logic or trying to add it. Either remove the caching claim or add a // TODO: implement revision-keyed caching note.

• src/ado/discovery.rs:69 — DiscoveryScope::Explicit is dead code

  The variant is defined, handled in apply_scope_filter, and exercised in unit tests, but no callsite ever constructs it. The --definition-ids path is handled entirely by the legacy resolve_definitions function before DiscoveryScope is ever created. Clippy may not catch this because the unit tests construct it — but it should at least get a comment (// Reserved for future use) or a #[cfg(test)] annotation on the test-only construction to make the intent explicit.

⚠️ Suggestions

• src/ado/discovery.rs:373 — PreviewError::NotFound maps to PreviewFailed

  A 404 from the Preview endpoint almost certainly means the definition was deleted in the window between list_definitions and preview_pipeline. It'll be emitted as a skipped_failed warning under --all-repos, confusing operators who'll see counts go up and then wonder why. Silently dropping 404s (or mapping them to a distinct DiscoveryStatus::NotFound that's excluded from the warning) would be more signal-correct.

• src/secrets.rs:41 — --source without --all-repos with no git remote gives a generic error

  When a user runs ado-aw secrets set KEY --source agents/foo.md outside a git repo, repo_url() returns None, apply_scope_filter returns an empty vec, and the user gets "No ado-aw pipelines found via Preview-driven discovery." — with no hint that the scoping culprit is the missing git remote. A targeted message like "Could not determine current repo URL; try adding --all-repos to search the full project" would be much friendlier.

✅ What Looks Good

• The marker-step approach is well-motivated and the empirical grounding (verifying ADO's Preview API strips top-of-file comments but preserves step body comments) is exactly the right level of rigor for a design decision like this.
• Security handling on the echo line is thorough: sanitize_for_vso_logging covers ##vso[ and ##[, bash_single_quote_escape uses the canonical '\'' idiom, and both have dedicated unit tests including an injection attempt.
• The parse_marker_step forward-compat behavior (unknown JSON fields silently ignored, malformed entries logged at debug and skipped rather than panicking) is the right default for a discovery surface that'll outlive compiler versions.
• The fast-path (local lock file → skip Preview HTTP call) is a nice latency win for the common case and the fallback-to-Preview design is sound.
• The 8-permit semaphore with ADO_AW_PREVIEW_CONCURRENCY env override is thoughtful — large projects won't accidentally hammer the ADO rate limiter.

Generated by Rust PR Reviewer for issue #624 · ● 736.3K · ◷

🔍 Rust PR Review

Summary: Solid implementation — the security work is careful and the architecture is well-thought-out. A few items worth discussing before merge.

Findings

🐛 Bugs / Logic Issues

• src/ado/discovery.rs:354 — normalize_repo_url doesn't normalize percent-encoding vs literal spaces. repo_url() percent-encodes project/repo names before the comparison, but if ADO's REST API ever returns a repository.url with spaces decoded (e.g. my project instead of my%20project), CurrentRepo filtering silently drops everything and --source without --all-repos returns zero results with no helpful error. The comment claims this was verified against one live endpoint; it's a latent assumption worth documenting as such, or adding a decode→reencode round-trip to make the comparison encoding-independent.

• src/ado/discovery.rs:565–571 — Consumer yaml_path is silently first-wins. discovered_to_matched sets yaml_path: d.markers.first().map(|m| m.source.clone()).unwrap_or_default(). For a Consumer that includes two templates (agents/a.md and agents/b.md), ado-aw list will consistently print → agents/a.md and silently omit agents/b.md. For secrets set, this means the displayed source reference in the summary may mislead the operator into thinking the variable was set only for one template when it was actually set on a pipeline that consumes both. The PR description acknowledges a richer surface is deferred, but the truncation isn't called out in the printed summary.

🔒 Security Concerns

• src/compile/extensions/ado_aw_marker.rs:1183 + src/ado/discovery.rs:565 — ##vso[ sanitization stops at the echo line. The echo correctly applies sanitize_for_vso_logging, but the raw source path flows unsanitized from the JSON comment → parse_marker_step → MarkerMeta(redacted) → MatchedDefinition::yaml_path→print_matched_summary(which writes to stdout). Ifado-aw secrets set --all-reposis ever called from an ADO pipeline step (e.g., in an operator automation pipeline), a crafted filename like##vso[task.setvariable variable=MY_SECRET]x.md` would pass through the JSON comment (bash comment, harmless there) but then surface in the CLI's own stdout, where the ADO agent would scan it. The threat is non-zero whenever the CLI is run inside a pipeline. Given the current operator-facing usage this is low risk, but it's architecturally inconsistent — the echo sanitizes but the downstream data flow doesn't.

⚠️ Suggestions

• src/ado/discovery.rs:282–286 — ADO_AW_PREVIEW_CONCURRENCY=0 is silently clamped. The .max(1) guard is correct and prevents a deadlock, but there's no warning when the user's env value parses to 0. A warn! or debug! log when the env value parses to a u64 but clamps to 1 would save future head-scratching.

• src/ado/discovery.rs — no test for --source with CurrentRepo scope when repository.url is absent. test_scope_current_repo_excludes_definitions_without_repository exists, but there's no test verifying that resolve_for_command bails with the helpful "no ADO git remote detected" message when repo_name is empty and source_filter is set. Currently that bail lives in secrets.rs and the discovery module only sees the scope enum — the integration path is untested.

✅ What Looks Good

• Security layering in the marker step is solid: sanitize_for_vso_logging + bash_single_quote_escape on the echo line, serde_json::json! for the JSON comment (properly encodes ", \, and special chars), and normalize_source_path strips \n/\r before anything is emitted. The attack paths for VSO command injection and bash quoting are well-covered where it matters most (the runtime surface).
• The DiscoveryStatus::NotFound variant correctly handles the TOCTOU race (definition deleted between list and preview) without surfacing a noisy error or inflating the skip counters.
• parse_marker_step is defensively written: trims leading whitespace (handles indented heredoc content), skips malformed JSON silently, accepts unknown fields via serde defaults, and is exercised by all the right edge cases.
• Semaphore permits.max(1) prevents a deadlock if a user sets ADO_AW_PREVIEW_CONCURRENCY=0.
• Fast-path lock-file lookup correctly normalizes paths (trim_start_matches('/') aligns with normalize_ado_yaml_path) so the optimization fires correctly in practice.
• CompileContext::new signature change (takes input_path instead of input_dir) is a nice cleanup — less footgun surface, and the parent().unwrap_or(".") fallback is now encapsulated in one place.

Generated by Rust PR Reviewer for issue #624 · ● 1.6M · ◷

🔍 Rust PR Review

Summary: Looks good overall — solid architecture, excellent test coverage, and careful attention to security. A few maintainability notes worth addressing before merge.

Findings

⚠️ Suggestions

• [src/compile/extensions/ado_aw_marker.rs + src/ado/discovery.rs] sanitize_for_vso_logging is duplicated

  The function is defined privately and identically in both new files:

  fn sanitize_for_vso_logging(s: &str) -> String {
      s.replace("##vso[", "[vso-filtered][")
          .replace("##[", "[filtered][")
  }

  src/sanitize.rs already handles VSO neutralisation (for markdown context), and src/agent_stats.rs has a sanitize_for_markdown with similar intent. If the set of dangerous prefixes ever needs to expand, this must be updated in two independent places with no compiler enforcement. Worth extracting to src/sanitize.rs as a pub function and importing from both call sites.

• [src/ado/discovery.rs:648-709] Side-effecting .filter() closure is confusing

  In resolve_definitions_via_discovery, the .filter() closure simultaneously decides inclusion and mutates four skip counters as a side effect. In the unfiltered case (normalized_filter.is_none()), it always returns true, so PreviewFailed/UnknownForbidden/UnknownRequiredParams items all land in kept and are only discarded later by discovered_to_matched. The variable name kept then misleads — it holds items that are about to be dropped. A two-pass approach (classify then warn then convert) or a manual for loop with explicit continue/push would be clearer and make it obvious that the warning counts and the final filtered set are derived from the same pass.

• [src/ado/discovery.rs:474-508] is_direct_match silently misclassifies sources with non-.md suffixes

  let Some(stem) = marker.source.strip_suffix(".md").map(|s| format!("{s}.lock.yml")) else {
      return false;
  };

  If marker.source doesn't end in .md (e.g. a future extension allows .yaml-sourced agents), the function returns false and the definition is classified as Consumer instead of Direct. The test fixture names document the convention, but there's no defensive log or comment explaining why a non-.md source is a legitimate case for returning false vs a marker corruption. Worth a // non-standard source extension; treat conservatively as Consumer comment at minimum.

✅ What Looks Good

• Security handling is thorough: both bash_single_quote_escape and sanitize_for_vso_logging are applied to the echo line before it reaches the generated bash. The JSON marker itself uses serde_json serialisation, so ", \n, and other special characters in source paths are correctly escaped. Injection tests exist for both vectors.
• Forward-compat serde on MarkerMetadata: #[serde(default)] on all fields means a schema-2 marker with new fields won't break the current parser, and a schema-1 parser encountering schema-2 output degrades gracefully.
• Fast-path for local lock files is a smart optimisation — skipping a network round-trip per already-compiled definition in the common case, while falling through to Preview as a defensive measure if the local parse fails.
• ADO_AW_PREVIEW_CONCURRENCY=0 warning (clamp-with-warn instead of silent clamp) is the right UX choice.
• Test coverage: 6 unit tests on the parser, 12 on discovery scope/classification logic, 4 integration tests for the marker across all four compile targets — comprehensive for a new infra feature.

Generated by Rust PR Reviewer for issue #624 · ● 1.4M · ◷

🔍 Rust PR Review

Summary: Solid implementation — the architecture is well-thought-out and the security mitigations are thorough. Two correctness issues worth fixing before merge.

Findings

🐛 Bugs / Logic Issues

1. normalize_source_path double-escapes " in JSON marker (src/compile/extensions/ado_aw_marker.rs:62–75)

normalize_source_path replaces " with \" for safe embedding in the YAML header comment (source="..."). But when the already-escaped string is then passed to serde_json::json!, serde serializes the backslash again, writing "source":"agents/foo\\\"bar.md" in the JSON rather than the correct "agents/foo\"bar.md".

// ado_aw_marker.rs
let source = super::super::common::normalize_source_path(input_path);
// ^ source = r#"agents/foo\"bar.md"# if path contained "
let metadata_json = serde_json::json!({
    "source": source,   // ← serde_json double-escapes the \
})
.to_string();

The round-trip still works in practice (the --source CLI path also goes through normalize_source_path, so both sides carry the extra backslash), but the JSON stored in every compiled lock file is semantically wrong. Fix: derive a separate forward-slash-only helper (no quote escaping) for the JSON metadata, and keep normalize_source_path for the YAML header comment only.

2. is_direct_match false-positive for same-stem paths in different directories (src/ado/discovery.rs:476–500)

yaml_normalized == stem || yaml_normalized.ends_with(&format!("/{stem}"))

stem for source = "agents/foo.md" is "agents/foo.lock.yml". The ends_with branch matches a definition whose yamlFilename is other/agents/foo.lock.yml — a completely unrelated pipeline with the same lock-file name in a different folder. The consequence is a Direct label on what is actually a Consumer (or unrelated) pipeline.

Low operational impact (write commands still act on the definition), but the mislabelling could confuse users reviewing --dry-run output. Consider requiring yaml_normalized == stem or a full-suffix check that includes the path separator on the left side too:

yaml_normalized == stem
    || yaml_normalized == format!("agents/{stem}")  // explicit repo-root prefix
    || yaml_normalized.ends_with(&format!("/{stem}"))
        && marker.source.contains('/')  // only accept when source itself is under a directory

Or use a stricter policy: only classify Direct via the local-lock fast path (which uses the exact yamlFilename from the definition), and classify all Preview-path matches as Consumer without trying to distinguish.

⚠️ Suggestions

3. --source matching is case-sensitive but undocumented (src/ado/discovery.rs:652)

d.markers.iter().any(|m| m.source == src)

On case-insensitive file systems (Windows), a user passing --source Agents/Foo.md would silently get zero results because the marker stores agents/foo.md. Either document this in the CLI help string, or normalise to lowercase before comparison (which normalize_source_path currently doesn't do).

4. revision field is stored but never read; the doc comment warns "no caching implemented yet" (src/ado/mod.rs:232–238)

The field is purely speculative infrastructure. No concern for this PR, just flagging so a follow-up #[allow(dead_code)] or // TODO doesn't get forgotten.

✅ What Looks Good

• Security mitigations are thorough: neutralize_pipeline_commands on the echoed source path, bash_single_quote_escape for the single-quoted bash string, and \n/\r stripping all being applied consistently. The unit test for ##vso[ injection directly validates the guard.
• Error handling follows project conventions: anyhow::bail! with actionable messages, .context() on HTTP calls, no unwrap() on user-facing paths outside of tests.
• The ADO_AW_PREVIEW_CONCURRENCY=0 deadlock guard (clamp to 1 with an explicit warn!) is a thoughtful safety catch that most implementations miss.
• The build_lock_path_map / normalize_ado_yaml_path key alignment is correct — both replace('\\', "/").trim_start_matches('/') — so the fast-path lookup works.
• Forward-compat #[serde(default)] on MarkerMetadata and skipping malformed JSON on parse_marker_step are exactly right for a schema that's expected to grow.
• The source_without_all_repos_bails_when_no_git_remote test is a good guard against the most likely user footgun.

Generated by Rust PR Reviewer for issue #624 · ● 1.8M · ◷

🔍 Rust PR Review

Summary: Looks good — well-designed feature with thorough tests and solid security thinking. A few minor issues worth addressing before merging.

Findings

🐛 Bugs / Logic Issues

• src/ado/discovery.rs:build_lock_path_map — ./-prefixed paths silently miss the fast-path

  build_lock_path_map strips leading / via trim_start_matches('/') but not leading ./. If detect_pipelines ever returns a path starting with ./ (e.g. ./agents/foo.lock.yml from a PWD-relative scan), the map key would be ./agents/foo.lock.yml but normalize_ado_yaml_path on the ADO yaml_filename side produces agents/foo.lock.yml. The fast-path lookup silently misses, and falls back to a Preview call. Not a correctness bug (fallback is fine), but degrades the intended optimization. The existing test returns PathBuf::from("pipeline.yml") so this doesn't bite in practice today, but it's fragile. Fix: add trim_start_matches("./") after the / trim, mirroring normalize_source_path.

• src/ado/discovery.rs:resolve_definitions_via_discovery — --source /abs/path.md silently matches nothing

  normalize_source_path strips ./ but not a leading /. A user passing --source /agents/foo.md (absolute path form) gets normalized_filter = Some("/agents/foo.md"), which never equals a marker's agents/foo.md. The CLI docs say to pass relative paths, but a silent no-match is worse than an error. Adding a leading-/ strip to normalize_source_path (or at the --source input site) would make this robust.

⚠️ Suggestions

• src/ado/discovery.rs:classify_definition — expect() in spawned task

  let _permit = sem.acquire_owned().await.expect("semaphore not closed");

  The semaphore is kept alive via Arc so this won't fire in practice, but a panic here propagates as a JoinError caught by Err(e) => warn!("Discovery worker panicked: {e}"), silently dropping that definition from results. The expect message is already descriptive; consider unwrap_or_else(|_| unreachable!("semaphore behind Arc cannot be closed")) or a comment making the invariant explicit. Very minor.

• src/ado/discovery.rs:is_direct_match — non-.md sources silently become Consumer

  The comment documents this as intentional ("conservative fallback"). Fine as-is, but worth a debug-log line so operators who accidentally compile from a .yaml source don't wonder why every definition shows up as Consumer.

• src/compile/extensions/mod.rs — AdoAwMarkerExtension in ExtensionPhase::Tool

  The marker has no tool semantics. ExtensionPhase::Setup would be a more accurate grouping and would naturally place the marker step before GitHub/SafeOutputs setup-phase steps in emit order. The current placement works because collect_extensions pushes AdoAwMarker first, but the ordering is then an implicit dependency on insertion order rather than phase.

✅ What Looks Good

• Security: The ##vso[ injection mitigation is thorough — both the echo line in the generated step (neutralize_pipeline_commands) and the yaml_path field in discovered_to_matched are sanitized. The distinction between why the JSON marker line doesn't need it (comments inside step bodies bypass the ADO logging-command scanner) is correctly documented and tested.
• normalize_source_path refactor: Removing " escaping from the shared helper and letting serde_json::json! handle JSON encoding is the right fix. The round-trip regression test (json_marker_quote_in_source_round_trips_correctly) catches the double-escape bug cleanly.
• normalize_repo_url: Percent-decode + ASCII-lowercase handles ADO's mixed-encoding URL forms correctly, and the explanation in the comment makes the non-obvious necessity clear.
• Error classification: PreviewError → DiscoveryStatus mapping is clean, and the distinct NotFound status for TOCTOU races avoids spurious "Preview Failed" warnings for definitions that just got deleted.
• ADO_AW_PREVIEW_CONCURRENCY=0 handling: Warn + clamp rather than silent deadlock avoidance. The .max(1) on the semaphore is redundant but harmless as a belt-and-suspenders guard.
• is_direct_match strict equality: The comment explaining why the earlier ends_with tail-match produced false-positives is exactly the kind of context that prevents the regression from coming back.
• Test coverage: 6 unit tests for parse_marker_step, 12 for scope/classification, 4 integration tests for per-target marker emission — comprehensive for the surface area changed.

Generated by Rust PR Reviewer for issue #624 · ● 1.9M · ◷

🔍 Rust PR Review

Summary: Looks good overall — well-designed feature with thoughtful security treatment, solid test coverage, and good documentation of migration trade-offs. A few items worth a second look.

Findings

⚠️ Suggestions

• src/ado/discovery.rs:732-743 — Skip-warning counters include non-ado-aw definitions in --all-repos mode.
  The skip counters (skipped_required_params, skipped_forbidden, skipped_failed) are incremented before the matches_filter check, so they count every failed definition in the project — including pipelines that aren't ado-aw at all (they'll show up as failed previews). In a large project with hundreds of non-ado-aw 400/403 responses, the warning "Discovery skipped N definitions whose Pipeline Preview requires templateParameters..." could be quite noisy and misleading. Consider only counting failures for definitions that would have been ado-aw candidates, or noting in the warning text that "N could include non-ado-aw pipelines."

• src/compile/extensions/mod.rs:130 — CompileContext::new silently uses Path::new("") when input_path has no parent.
  input_path.parent() for a bare filename "foo.md" returns Some(Path::new("")), not None, so the unwrap_or(Path::new(".")) fallback is never reached in that case. Path::new("") and Path::new(".") behave differently when running git -C "" vs git -C . in infer_ado_context. This is pre-existing (callers previously did the same parent().unwrap_or(".") themselves), but worth a comment noting the Some("") behaviour since it's now hidden inside the function.

• src/ado/discovery.rs:534 — marker_origin_matches when current_org resolves to "".
  In resolve_definitions_via_discovery, if ctx.org_name() returns None (org_url has no path segment), current_org becomes "". A strict marker (non-empty org/repo) would then fail the eq_ignore_ascii_case("") check and be silently excluded. In practice this requires --source --all-repos with a malformed/missing ADO context (which should also fail auth), but there's no test for this case and no error surfaced to the user. At minimum a debug! log when current_org is empty while a source filter is active would help diagnose silent mismatches.

• src/compile/extensions/ado_aw_marker.rs — normalize_source_path with absolute input paths.
  If a user invokes ado-aw compile /absolute/path/to/agents/foo.md, the marker stores the full absolute path (/absolute/path/to/agents/foo.md). A --source agents/foo.md filter would then never match. normalize_source_path strips ./ but not absolute path components. This is pre-existing behaviour (not a regression), but worth a doc note or validation in the --source handler since the mismatch is silent.

✅ What Looks Good

• Security sanitization is thorough and correct. The ##vso[ neutralisation in the echo line, the bash_single_quote_escape idiom, and the discovered_to_matched yaml_path sanitisation are all well-motivated with tests that verify the injection defence. The analysis in the comments that the JSON # ado-aw-meta(redacted) comment is never sent to stdout by bash (only the echo` line is) is accurate.
• The normalize_source_path double-escaping fix is a real correctness fix. Feeding the raw path into serde_json::json! and letting serde_json handle JSON-level escaping (rather than pre-escaping " and getting the backslash double-encoded) is the right approach, and the round-trip test json_marker_quote_in_source_round_trips_correctly pins it down nicely.
• is_direct_match equality-over-suffix fix. The regression test consumer_when_same_stem_in_different_directory captures exactly the case that was broken and clearly explains why ends_with was wrong — good defensive documentation.
• 1ES generate_setup_job now routes extension setup_steps — previously the only compile target that didn't, meaning the always-on marker would have been silently absent in 1ES-compiled pipelines without this fix.
• Test coverage is comprehensive. Unit tests for parse_marker_step, is_direct_match, apply_scope_filter, marker_origin_matches, normalize_repo_url, and discovered_to_matched cover all the meaningful edge cases. The integration tests asserting exactly one marker per compiled target are the right way to regression-pin the extension wiring.

Generated by Rust PR Reviewer for issue #624 · ● 1.3M · ◷

🔍 Rust PR Review

Summary: Looks good — well-designed feature with strong test coverage. A few items worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/ado/discovery.rs:build_lock_path_map vs normalize_ado_yaml_path divergence risk — build_lock_path_map builds keys by hand (replace('\\', "/") + trim_start_matches('/')) while the lookup side uses super::normalize_ado_yaml_path. These are semantically equivalent today but will silently diverge if normalize_ado_yaml_path ever gains additional canonicalisation (e.g. lowercasing or URL-decoding). Using the shared helper for both sides would make this future-proof:

  let key = super::normalize_ado_yaml_path(&normalized.trim_start_matches('/'));
  map.insert(key, path.clone());

• src/ado/discovery.rs:resolve_definitions_via_discovery — uninspectable definitions land in selected for --all-repos alone — When normalized_filter is None, matches_filter is true for every definition including UnknownRequiredParams, UnknownForbidden, and PreviewFailed ones. They get pushed into selected and then discarded by discovered_to_matched. For a large project this bloats the intermediate vec. Not a correctness bug (the counters and final output are both correct), but an easy win: push only when both matches_filter and the status is actionable, or push first and let discovered_to_matched do the dual duty it already does. Either way the counts must still be tallied before the push guard.

⚠️ Suggestions

• src/compile/extensions/ado_aw_marker.rs — missing trailing newline — The file ends with } without a final newline (\ No newline at end of file in the diff). cargo fmt will flag this; easy fix.

• src/ado/discovery.rs:is_direct_match — heuristic boundary worth a comment — markers.len() > 1 as a "Consumer" signal is correct for the current architecture (a Direct pipeline's expanded YAML has exactly one marker from its own Setup job). But the function doc doesn't explain why > 1 implies Consumer. A one-liner would help future readers who wonder whether this was a deliberate design or an arbitrary threshold:

  // A compiled ado-aw pipeline's expanded YAML carries exactly one marker
  // (its own Setup-job step). More than one means the YAML was produced by
  // expanding a consumer that includes multiple templates — none of which
  // are this definition's "own" root YAML.
  if markers.len() > 1 {
      return false;
  }

• src/ado/discovery.rs:resolve_definitions_via_discovery — --source without --all-repos missing hint on empty match — When the discovery path returns Ok(Some([])) for a --source filter scoped to CurrentRepo, the empty-match bail in run_set/run_list/run_delete says "No ado-aw pipelines found via Preview-driven discovery." but doesn't tell the user whether the template simply has no consumers in this repo or whether they forgot --all-repos. A small hint like "...try --all-repos to search the entire project" would match the quality of the other error messages.

✅ What Looks Good

• Double-escape regression fix: removing the replace('"', "\\\"") from normalize_source_path and letting serde_json::json! handle JSON encoding is the correct approach — the old path would have stored \" in the JSON which serde then re-escaped to \\". The regression test (json_marker_quote_in_source_round_trips_correctly) pins this cleanly.
• ##vso[ injection defence in depth: the echo line goes through neutralize_pipeline_commands and bash_single_quote_escape, with a targeted test for each threat. The discovered_to_matched neutralization on the yaml_path field adds another layer for stdout surfaces.
• Semaphore env-tunability with deadlock-avoidance warning: clamping ADO_AW_PREVIEW_CONCURRENCY=0 to 1 with a warn! (rather than silently using 0 and deadlocking) is the right UX choice.
• MarkerMetadata forward-compat design: serde(default) on all fields + skip-on-malformed-JSON in parse_marker_step means future schema additions won't break existing callers. The test_parse_marker_step_tolerates_extra_json_fields test correctly pins this.
• CompileContext::new parent-dir fix: the Some(p) if !p.as_os_str().is_empty() guard for bare filenames returning "" from Path::parent() is subtler than the old unwrap_or(Path::new(".")) and the comment explaining why is valuable.
• Test coverage: 12 unit tests in discovery.rs, 6 in detect.rs, 4 integration tests across all four compile targets, and two async precondition tests in secrets.rs — comprehensive for the surface area changed.

Generated by Rust PR Reviewer for issue #624 · ● 721.4K · ◷

🔍 Rust PR Review

Summary: Looks good — solid implementation with thorough tests. A few minor items below, none blocking.

Findings

⚠️ Suggestions

• ado_aw_marker.rs — PR description mischaracterizes the hook used.
  The description and PR body say "via the existing CompilerExtension::setup_steps hook" and "Setup-job bash step", but the actual implementation correctly uses fn prepare_steps(...) (Agent job prepare phase). The code comment explains the deliberate choice: a setup job would spin up an extra pool agent just for a metadata comment. The mismatch only matters for future contributors reading the description, but AGENTS.md carries a similar "Setup-job" description in its stored-memory block — worth a one-liner correction there to avoid confusion.

• src/ado/discovery.rs — DiscoveryScope::Explicit is unreachable dead code.
  The variant is documented "Reserved for future use", but no production callsite constructs it, and --definition-ids short-circuits before discovery is invoked. Since clippy is reportedly clean, there must be an #[allow(dead_code)] somewhere (or it's pub so the compiler doesn't warn). Minor, but if this variant is truly reserved-for-future and not tested, a // TODO or a #[doc(hidden)] might communicate the intent more clearly than leaving it silently unreachable.

• src/ado/discovery.rs:classify_definition — Direct vs Consumer mislabelling edge case.
  is_direct_match uses the heuristic: exactly 1 marker + yamlFilename == stem + ".lock.yml". A hand-crafted pipeline that (a) includes exactly one ado-aw template and (b) happens to share the same root filename as that template's lock file would be labelled Direct instead of Consumer. In practice this is purely cosmetic — both statuses are actionable and reach discovered_to_matched — but the classification would be misleading in the CLI summary. Low probability scenario, worth a comment in is_direct_match acknowledging the cosmetic impact.

• src/ado/discovery.rs — large-project task explosion.
  handles.push(tokio::spawn(...)) is called for every filtered definition before any handle is awaited. The semaphore correctly limits concurrent HTTP calls to 8, but in a project with thousands of definitions (e.g., msazuresphere/4x4), all N tokio tasks are spawned upfront. Tasks are cheap but not free; worth a comment noting the known trade-off. If N could be very large, an alternative is a stream-based approach (FuturesUnordered with bounded concurrency). Not urgent at current project scales.

✅ What Looks Good

• Security coverage is thorough. ##vso[ injection is neutralised at both the echo line in the marker step (neutralize_pipeline_commands + bash_single_quote_escape) and at the summary output path in discovered_to_matched. Both have specific regression tests.

• serde_json::json! for the metadata blob — correctly hands JSON-level escaping to serde rather than manually embedding the source path, preventing double-escaping. The json_marker_quote_in_source_round_trips_correctly test explicitly catches the previous regression.

• normalize_repo_url percent-decodes before case-folding — robustly handles both ADO-encoded (My%20Project) and decoded forms in URL comparison.

• parse_marker_step is forward-compatible — missing/unknown JSON fields are silently ignored (no #[serde(deny_unknown_fields)]), and the test_parse_marker_step_tolerates_extra_json_fields test confirms it.

• marker_origin_matches lenient-for-empty — correctly falls back to source-only matching for legacy markers (empty org/repo) so pre-marker pipelines remain discoverable during the transition window.

• Error propagation — anyhow::Result throughout, .context() on every HTTP call, descriptive bail! messages at CLI boundary. No user-facing unwrap() outside provably-safe cases.

• 12 unit tests for pure discovery logic, 6 for parse_marker_step, 4 integration tests for the marker step across all four targets — good coverage depth.

Generated by Rust PR Reviewer for issue #624 · ● 1.1M · ◷

🔍 Rust PR Review

Summary: Solid implementation — well-structured, good security hygiene, and comprehensive test coverage. No blocking issues found.

Findings

✅ What Looks Good

• is_direct_match strict equality: The earlier ends_with("/{stem}") tail-match is correctly replaced with strict equality after normalize_ado_yaml_path. The regression test (consumer_when_same_stem_in_different_directory) explicitly covers the false-positive case that motivated the change.

• ##vso[ injection in echo step: The injected marker step correctly applies neutralize_pipeline_commands to the echo's user-controlled fields (source, org, repo) and bash_single_quote_escape for bash quoting safety. The JSON comment line is correctly left unescaped since ADO's logging-command scanner only fires on runtime stdout, not on comment lines rendered from the step body.

• normalize_source_path double-escape fix: Removing replace('"', "\\\"") from the shared helper and letting serde_json::json!() handle JSON-level escaping is the right fix. generate_header_comment correctly applies its own inline replace('"', "\\\"") for the YAML-comment surface. The round-trip test (json_marker_quote_in_source_round_trips_correctly) closes the regression.

• Concurrency guard: The ADO_AW_PREVIEW_CONCURRENCY=0 deadlock guard with an explicit warn! (rather than silent .max(1)) is the right call — it makes the typo debuggable.

• clap conflicts_with: --all-repos and --source both carry conflicts_with = "definition_ids", so the claimed mutual exclusivity is enforced at parse time rather than silently resolved in code.

• TOCTOU race (definition deleted mid-discovery): Correctly tracked as NotFound (distinct from PreviewFailed) so it's excluded from the "skip warning" count without requiring operator action.

• Legacy path preserved: resolve_for_command only activates discovery when definition_ids.is_none() && (all_repos || source_filter.is_some()), leaving the legacy lexical matcher as the default for all existing call sites.

⚠️ Suggestions

• src/ado/discovery.rs:144 — sem.acquire_owned().await.expect("semaphore not closed") inside a tokio::spawn closure: if this expect ever fires (unreachable given the Arc, but defensively), the spawned task panics, handle.await returns Err(JoinError), and the definition is silently dropped with only a warn!("Discovery worker panicked"). No caller would know a definition was skipped. Consider replacing with sem.acquire_owned().await.unwrap_or_else(|_| panic!(...)) or — more robustly — returning an Err from the closure so the outer loop can propagate or surface it more explicitly. Low priority given the semaphore is Arc-wrapped and cannot be closed before the tasks finish, but worth hardening before any future refactor that might move the drop point.

• src/ado/discovery.rs (resolve_definitions_via_discovery doc) — the doc says --source matching "is case-sensitive even on Windows". This is correct and intentional, but it means a user who passes agents\Foo.md on Windows (wrong case) silently matches nothing, even after normalize_source_path has fixed the separator. The CLI help already calls this out, so this is more of a UX note — consider whether a case-fold fallback with a warning would improve the experience, especially for Windows users piping from Explorer.

• src/ado/discovery.rs:revision field — revision: Option<u64> is deserialized on DefinitionSummary but the doc comment explicitly says "No caching is implemented yet." Consider marking it #[allow(dead_code)] or a TODO comment with an issue reference so the field doesn't quietly accumulate API data that future contributors assume is being used.

Generated by Rust PR Reviewer for issue #624 · ● 1.7M · ◷